WIP: Add useful DT spec macros to unify clock control consumer codes and remove coupling of specific drivers. Converting NXP platforms WIP #96315
Conversation
Add generic macros for clock consumer to get arbitrary data from DT clocks specifiers that doesn't need to leak implementation details. This is an attempt of unification of clock control drivers but will require clock control drivers to actually all switch to using the clock_control_dt_spec as their clock_control_subsys_t, and for clock consumers to actually use the opaque macros. However, nobody is actually forced to use these macros (at least, not any more than they are already forced to couple to driver-specific subsys structure), it's just they should switch if they want to reap the benefit of the unification. Signed-off-by: Declan Snyder <[email protected]>
Convert NXP clock control drivers and clock consumer drivers to the new unified macros. This has to be all in one commit because it is very hard to make it bisectable otherwise. This is because many of the drivers are actually benefitting from these macros due to they have a bunch of code that is coupling to multiple different consumers and so forth. NOTE: right now this commit is only containing clock driver changes, not consumers. And they are not finished Signed-off-by: Declan Snyder <[email protected]>
there might be some copy paste mistakes and stuff, it's just a concept commit at the moment this one will be squashed into the previous one for bisectability
decsny
requested review from
DerekSnell,
EmilioCBen,
butok,
danieldegrasse,
dleach02,
mathieuchopstm and
mmahadevan108
|
decsny
changed the title
| .base = (UART0_Type *)DT_INST_REG_ADDR(n), \ | ||
| .clock_dev = DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(n)), \ | ||
| .clock_subsys = (clock_control_subsys_t)DT_INST_CLOCKS_CELL(n, name),\ | ||
| .clock_subsys = CLOCK_CONTROL_DT_SPEC_INST_GET(n, clocks), \ |
There was a problem hiding this comment.
So would the idea then be that each clock driver would know how to handle this data, and the format of the data would be set entirely in devicetree (therefore we don't have any coupling between the SOC clock driver and clock consumer)?
If so, I think this definitely sense as a method for static setup. I'll also mention that the STM32 static configuration is (IMO) the gold standard right now for how to do it in tree. They essentially use the clock control driver's init phase to apply settings like PLL and multiplexer configuration, and then each individual driver handles things like enabling its clock gate.
There was a problem hiding this comment.
yes look for example at the syscon lpc driver which I mostly got working on this PR so far (hello world runs on RW612 last time I checked).
The great thing about refactoring all the clock consumers and drivers to use this macro and just get the raw data from DT in a generic manner, is that now, if I want to for example change the LPC syscon binding to have different cells and completely change the driver implementation to be less stupid (you know what I'm talking about) then I don't have to change any clock consumer's code to do it. As a matter of fact, I can even change those platforms so that there is not a "syscon" driver anymore and in fact there could be multiple clock control drivers/bindings and I still don't have to change the clock consumer code. Thats because they are no longer coupled to specific clock control driver implementations.
So see, we can solve the original problem without a gigantic architectural rework, just a few macros that everybody unifies under.
So as far as your RFC, the point I want to say is that it may be trying to solve other problems, but at least de-coupling consumer drivers from clock driver implementations specifically can be done easily (a lot of trivial work is all) right now without a new subsystem.
There was a problem hiding this comment.
Also, the clock control API right now has an opaque pointer for setting rate, which it would be interesting to see if a very similar thing can be done in order to provide things like constraints or whatever from DT. And probably an error code like -EBUSY can be added to the clock_control_set_rate and clock_control_configure APIs if the rate or configuration can't be done due to something else already configured it in an incompatible way. This I know is a very simple mechanism probably not as advanced as what you have in your PR but it would a good starting place IMO to see what we are actually missing. I am not going to try to do that in this PR though, I am only interested in decoupling the clock specifiers right now, not optimizing the clock control drivers or dynamic changes yet.
And clock control drivers right now could be implemented more hierarchically without having to change the API I think. A clock control node can have a clocks property of it's own to refer to yet another clock control driver (such as a PLL driver or whatever). What you described about the STM32 driver still sounds pretty limited but I haven't looked into it.
|
This is very similar to what was proposed in this talk: https://www.youtube.com/watch?v=VKMQ-jzS2cw I was almost sure there was a PR associated to it, but I'm unable to find it - all I managed to find was #46425, which is somewhat of a mix between that and a "mini CMS". |
mathieuchopstm
left a comment
mathieuchopstm
left a comment
There was a problem hiding this comment.
Seems acceptable overall, and this could definitely be merged as-is in tree since it should not cause any breakage (as long as no consumer is updated without the producer being updated too).
One small downside on footprint side is that the full range of uint32_t is not usually consumed by single cells; custom macros allowed packing N cells in a single u32, which is not possible here. But I'd argue that's a small price to pay... and at worse, cells can be changed to perform packing at DT level (c.f. how it's done in STM32)
| /* This is a useful way to encode opaque data, this is private structure not part of API */ | ||
| struct clock_control_dt_spec { | ||
| uint32_t *cells; /* the cells in the DT phandle specifier */ | ||
| size_t len; /* the number of cells in the DT phandle specifier */ | ||
| }; | ||
|
|
||
| #define ZPRIV_CLOCK_CONTROL_DT_DEFINE(node_id, prop, idx) \ | ||
| static uint32_t _CONCAT_6(zpriv_clock_control_cells_, node_id, _, prop, _, idx) = { \ | ||
| DT_FOREACH_PHA_CELL_BY_IDX_SEP(node_id, prop, idx, DT_PHA_BY_IDX, (,)) \ | ||
| }; \ | ||
| static struct clock_control_dt_spec \ | ||
| _CONCAT_6(zpriv_clock_control_, node_id, _, prop, _, idx) = { \ | ||
| .cells = \ | ||
| &(_CONCAT_6(zpriv_clock_control_cells_, node_id, _, prop, _, idx)), \ | ||
| .len = DT_PHA_NUM_CELLS_BY_IDX(node_id, prop, idx), \ | ||
| } /* intentionally require semicolon */ |
There was a problem hiding this comment.
I would inline the cells array to save some space:
| /* This is a useful way to encode opaque data, this is private structure not part of API */ | |
| struct clock_control_dt_spec { | |
| uint32_t *cells; /* the cells in the DT phandle specifier */ | |
| size_t len; /* the number of cells in the DT phandle specifier */ | |
| }; | |
| #define ZPRIV_CLOCK_CONTROL_DT_DEFINE(node_id, prop, idx) \ | |
| static uint32_t _CONCAT_6(zpriv_clock_control_cells_, node_id, _, prop, _, idx) = { \ | |
| DT_FOREACH_PHA_CELL_BY_IDX_SEP(node_id, prop, idx, DT_PHA_BY_IDX, (,)) \ | |
| }; \ | |
| static struct clock_control_dt_spec \ | |
| _CONCAT_6(zpriv_clock_control_, node_id, _, prop, _, idx) = { \ | |
| .cells = \ | |
| &(_CONCAT_6(zpriv_clock_control_cells_, node_id, _, prop, _, idx)), \ | |
| .len = DT_PHA_NUM_CELLS_BY_IDX(node_id, prop, idx), \ | |
| } /* intentionally require semicolon */ | |
| /* This is a useful way to encode opaque data, this is private structure not part of API */ | |
| struct clock_control_dt_spec { | |
| size_t len; /* the number of cells in the DT phandle specifier */ | |
| uint32_t cells[]; /* the cells in the DT phandle specifier */ | |
| }; | |
| #define ZPRIV_CLOCK_CONTROL_DT_DEFINE(node_id, prop, idx) \ | |
| static uint32_t _CONCAT_6(zpriv_clock_control_cells_, node_id, _, prop, _, idx) = { \ | |
| \ | |
| }; \ | |
| static struct clock_control_dt_spec \ | |
| _CONCAT_6(zpriv_clock_control_, node_id, _, prop, _, idx) = { \ | |
| .len = DT_PHA_NUM_CELLS_BY_IDX(node_id, prop, idx), \ | |
| .cells = { DT_FOREACH_PHA_CELL_BY_IDX_SEP(node_id, prop, idx, DT_PHA_BY_IDX, (,)) } \ | |
| } /* intentionally require semicolon */ |
| nxp_references))), (NULL)),\ | ||
| .clock_dev = DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(n)), \ | ||
| .clock_subsys = (clock_control_subsys_t)DT_INST_CLOCKS_CELL(n, name),\ | ||
| .clock_subsys = CLOCK_CONTROL_DT_SPEC_INST_GET(n, clocks),\ |
There was a problem hiding this comment.
Considering 99.99% of uage will be with with prop=clocks, I'd make specialized versions of the macro that include it directly, e.g.
#define CLOCK_CONTROL_DT_SPEC_INST_GET_DEFAULT(inst) /* ... */| .clock_subsys = CLOCK_CONTROL_DT_SPEC_INST_GET(n, clocks),\ | |
| .clock_subsys = CLOCK_CONTROL_DT_SPEC_INST_GET_DEFAULT(n),\ |
Ditto for DEFINE.
There was a problem hiding this comment.
Originally when I was making this PR, I did do something about this where you optionally don't have to say "clocks", by using variable arguments but I decided for first pass to just be explicit
| struct lpc_syscon_spec { | ||
| uint32_t name; | ||
| }; | ||
|
|
||
| static int lpc_syscon_get_spec(clock_control_subsys_t subsys, struct lpc_syscon_spec *spec) | ||
| { | ||
| struct clock_control_dt_spec *dt_spec = subsys; | ||
|
|
||
| if (dt_spec->len != 1) { | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| spec->name = *(dt_spec->cells); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Just to be clear: this is merely for example, and it would be fine for drivers to do instead
+#define LPC_CELL_NAME 0
static int mcux_lpc_syscon_clock_control_on(const struct device *dev,
clock_control_subsys_t sub_system)
{
#if defined(CONFIG_CAN_MCUX_MCAN)
- if ((uint32_t)sub_system == MCUX_MCAN_CLK) {
+ if (sub_system->cells[LPC_CELL_NAME] == MCUX_MCAN_CLK) {
CLOCK_EnableClock(kCLOCK_Mcan);
}
#endif /* defined(CONFIG_CAN_MCUX_MCAN) */for example?
There was a problem hiding this comment.
The drivers can do whatever they want. This lpc syscon driver right now is really bad right now and probably needs the DT binding actually changed and driver optimized so that it isn't 5 million ifs, and is some more useful info instead. The benefit of unifying under these macros is that type of change can be done without having to edit any clock control consumer's code. And nxp has like 10 clock control drivers so I will be leaving these optimization activities to other NXP developer, but right now I'm trying to make it so that they can do that work without all of them having to update 200 consumer drivers for each PR
|
@mathieuchopstm another alternative I considered is to do something similar to the pinctrl scheme with pinctrl_soc.h where the platform implement some like Z_CLOCK_CONTROL_DEFINE macro so that some optimization can be done, instead of describing the format here, what do you think |
The benefit of this unification is that you can change the DT descriptions and clock control drivers implementations without having to change any consumer drivers - so if you want to pack more meaning into your cells it would be very easy to do that , all you would have to do is edit the clock control driver, binding, and the relevant DTS files and macros. There is nothing saying, each discrete piece of information needs to be a whole 32 bit cell. It's completely generic and arbitrary, the bindings can do whatever they want. |
Yes, that is what I was implying by saying:
Not against it but this sounds like it would require more changes (i.e., create new header files, update If I understand correctly, this would also mean the Clock Control API would become unique-per-SoC: while I don't think there are much implementations of this in tree, it does mean that external generators could no longer be represented using this API too? |
I actually could keep it like it is and check if a the "custom implementation macro" is defined, then use that, instead of using the default dt_spec iterator one here, that's best of both worlds I guess.
I'm not sure what you mean by this |
Even though I don't think there is any such usage in tree, I think the Clock Control is supposed to also be implemented by (external) clock generators. If we move towards a |
You are right, that would be an issue. It is also an issue with pinctrl, actually. So nevermind, I will stick with current approach. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
@ZhaoxiangJin suggest taking at look at this approach in case the clock management rework stuff is taking too long, if you go with this idea then it's just a trivial mechanical coding exercise to refactor everything so you don't have to edit clock control drivers for every single time you add/change a non-clock driver which would make developing the NXP drivers a lot simpler. If have any question LMK. |
Thank you @decsny, you make thinks better, wishing you all the best in your next chapter. |
4 participants
Pretty much what the title says, it's very very early WIP but just showing concept, don't mind the stupid code mistakes or incompleteness, I'm going to cancel the twister run